Skip to content
This repository was archived by the owner on Sep 12, 2019. It is now read-only.

Condense element types - remove EmbeddedTemplate and EmbeddedContent #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Condense element types - remove EmbeddedTemplate and EmbeddedContent #34

wants to merge 4 commits into from

Conversation

@mk13
Copy link
Contributor

@mk13 mk13 commented Apr 18, 2017

This is a pretty big design change - I've decided to essentially remove 'EmbeddedTemplateAst' and 'EmbeddedContentAst'.

TL;DR: Having three types had no purpose and just overcomplicated things.

Reasoning:

  • At a class level, these two weren't doing anything unique. They were both just essentially a 'gutted' version of 'ElementAst' with different class names. They're all just containers to begin with.
  • Instead of using the class name to differentiate, just use a simple bool flag within the ElementAst (isTemplate and isNgContent).
  • Building from above, this allows for a 'common type' where users won't have to always check if the type is ElementAst, EmbeddedContentAst, or EmbeddedTemplateAst. All the appropriate irrelevant fields are already emptied out (Example: if name is 'ng-content', bananas, events, properties, etc. are all empty lists).

@ferhatb
Copy link

ferhatb commented Apr 18, 2017 via email

@mk13
Copy link
Contributor Author

mk13 commented Apr 18, 2017

I'll take a look at the AngularDart code to see how they distinguish these types and see how their visitors behave.

@MichaelRFairhurst
Copy link
Contributor

MichaelRFairhurst commented Apr 18, 2017

Its also possible to create a base/mixin class which distinguishes these, ie,

abstract class ElementTypeDistinguishingVisitorMixin implements AngularAstVisitor {
  void visitElementAst(ElementAst ast) {
    if (ast.name == 'template') {
      visitEmbeddedTemplateAst(ast);
    } else if (ast.name == 'ng-content') {
      visitEmbeddedContentAst(ast);
    } else {
      visitNormalElementAst(ast);
    }
  }

  void visitEmbeddedTemplateAst(ElementAst ast);
  void visitEmbeddedContentAst(ElementAst ast);
  void visitNormalElementAst(ElementAst ast);
}

class PreexistingAngularVisitor with ElementTypeDistinguishingVisitorMixin {
   // hopefully relatively few changes here
}

Alternatively, all three classes could share an interface and we could use

abstract class HandleEmbeddedContentLikeNormalVisitorMixin implements TemplateAstVisitor {
    void visitCommonElement(CommonElementAst ast);

    void visitElementAst(ElementAst ast) {
      visitCommonElement(ast);
    }

    void visitEmbeddedContentAst(EmbeddedContentAst ast) {
      visitCommonElement(ast);
    }

    void visitEmbeddedTemplateAst(EmbeddedTemplateAst ast) {
      visitCommonElement(ast);
    }
}

and alternatively alternatively, we can use composition over inheritance if we want to prevent accidental overriding of the special mixin methods. (the "normalizing" visitor is a regular visitor, but accepts a "normalized" visitor which has no method visitElementAst, only a method visitCommonElement, and the normalizing visitor just forwards the visitations)

@mk13
Copy link
Contributor Author

mk13 commented Apr 18, 2017 •
edited
Loading

I looked through the visitors in AngularDart and it seems that the only visitor that does things differently for the three element types is only in ViewBuilderVisitor.

The others (ViewBinderVisitor, TemplateHumanizer, TemplateContentProjectionHumanizer) have a lot of overlapping behavior within all three types (ElementAst, NgContent, EmbeddedTemplateAst) and can realistically be merged into a single visitor (visitElementAst) that still maintains the intended behavior.

If there are no plans to expand the number of visitors, then the Mixin might even be overkill since it will only realistically be applied to ViewBuilderVisitor.

@mk13 mk13 mentioned this pull request Apr 18, 2017
Merged
@ferhatb
Copy link

ferhatb commented Apr 19, 2017 via email

MichaelRFairhurst
MichaelRFairhurst approved these changes May 1, 2017
View reviewed changes
Copy link
Contributor

@MichaelRFairhurst MichaelRFairhurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for blanking on this for so long. Seems I missed the moment when the original question was answered.

Can't argue its a nice reduction of code!

Did have some structural thoughts, but I think I might leave such structural decisions overall to you and @matanlurey . I can see some value in keeping things a bit more distinct...but definitely to me this looks like a quite nice maintanability change.

lib/src/parser/recursive.dart

if (isTemplateElement) {
return new EmbeddedTemplateAst.parsed(
if (isNgContent) {
Copy link
Contributor

@MichaelRFairhurst MichaelRFairhurst May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using a subtyping pattern here?

class NgContentElement extends Element {

  void reportChildNode(Element childNode, ExceptionHandler handler) {
    // NgContentElement.reportChildNode throws while Element and TemplateElement accept it
  }
  ...
}

would remove some of the else-ifiness from the code. Although I suppose it would once again that the instantiation of these would have differences....Hmm, for that, maybe you could make a factory for Element.

factory Element(String tagname, ...) {
   if (tagname == "template") {
     return new TemplateElement(tagname, ...);
   } else ...

Copy link
Contributor Author

@mk13 mk13 May 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried the sub-typing pattern before but it still causes the issue of Element potentially being also either TemplateElement or NgContentElement - and we would just have to check those again.

I haven't tried the factory solution yet, but will try once this issue is picked up again.

lib/src/parser/recursive.dart
references: references,
bananas: bananas,
stars: stars,
stars: (uniqueStarAst == null ? [] : [uniqueStarAst]),
Copy link
Contributor

@MichaelRFairhurst MichaelRFairhurst May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to have all the star asts inside the ElementAst, so that we can, for instance, autocomplete inside them before forcing the user to refactor into two elements with stars.

Copy link
Contributor Author

@mk13 mk13 May 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea. I'll probably incorporate this in a separate PR with all the necessary checks needed for this.

@mk13
Copy link
Contributor Author

mk13 commented May 5, 2017

Putting this PR onto a freeze until more PoC can be done on how well it can integrate into angular2.

@kevmoo
Copy link
Contributor

kevmoo commented Oct 13, 2017

Guessing we can close this down, right?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@matanlurey matanlurey matanlurey approved these changes

@MichaelRFairhurst MichaelRFairhurst MichaelRFairhurst approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mk13 @ferhatb @MichaelRFairhurst @kevmoo @matanlurey @googlebot